Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added local functions to dbauth #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

gregvis
Copy link
Contributor

@gregvis gregvis commented May 27, 2016

i was calling superlogin.removeExpiredKeys() and it was failing when trying to call self.removeKeys(). I noticed that removeKeys was not part of the superlogin object, so I made it a local function instead since it didn't require the superlogin object for anything. I ended up making 5 of the functions local, but I also exported them using "this" just in case they are being called by another module. If they are not being called by another module, then they can just be private functions. I did not have time to check if they were being referenced by other modules.

@colinskow
Copy link
Owner

Thank you very much! I will merge this within the next few days. Bonus points if you can expand the tests to ensure these all work and won't break in the future.

@gregvis
Copy link
Contributor Author

gregvis commented May 30, 2016

I've added "it should delete all expired keys" to test.js which verifies that superlogin.removeExpiredKeys() works properly.

@colinskow
Copy link
Owner

Awesome... thank you very much!!! This will definitely go into the next release.

@clariontools
Copy link

clariontools commented Oct 1, 2017

@colinskow -

Awesome... thank you very much!!! This will definitely go into the next release.

Looks like this never made it into the next release, will it ever make it? Or maybe there is an example of calling superlogin.removeExpiredKeys() in an easy way without incorporating these modifications? The README.md says...

For additional security, periodically run superlogin.removeExpiredKeys() either with setInterval or a cron job. This will deauthorize every single expired credential that exists in the system.

However there is no example to follow and since removeKeys() is not part of superlogin it is not accessible and throws the "self.removeKeys is not a function" error, making it unusable as is from server.js. Does anyone have an example of using superlogin.removeExpiredKeys() without these proposed changes?

EDIT: looks like on January 24, 2017 SukantGujar Assign pre-bound removeExpiredKeys commit was applied to user.js that may address this issue in a different way. The issue now is that at least for me doing a simple:

npm install superlogin

does not seem to install the latest commits? That may be intentional and I also noted that the most recent commit for the pouchdb version update did not install for me, so not sure what the problem is. I suppose I will have to clone the repo or just the files that have been modified but do not install using npm install superlogin. Maybe a note could be added to the readme or some sort of confirmation about if there is a problem or how to use the latest?

EDIT: As expected the January 24, 2017 SukantGujar Assign pre-bound removeExpiredKeys commit fixes the problem that this PR attempted to address. If anyone ends up here, you can pull down the January 24, 2017 SukantGujar Assign pre-bound removeExpiredKeys commit, but as of October 1, 2017, npm install superlogin does not include the fix. Looks like other fixes as well are not included in the npm install, maybe the best is to just clone the repo until all the issues are verified.

@micky2be
Copy link
Contributor

micky2be commented Oct 2, 2017

Yes sadly the last release on NPM is 0.6.1.
We need contributors here as @colinskow seems busy on other projects.

If needed you can install modules from GitHub using "superlogin": "colinskow/superlogin#commit_hash" in your package.json

The command line npm install --save colinskow/superlogin#commit_hash should work too I believe.

@clariontools
Copy link

@micky2be - Thanks for the confirmation. I am not familiar with what steps are required to update the official NPM package, but did not think it was that difficult to get done. I may be able to help out as a contributor, let me know what the process is to get access.

@repjov
Copy link

repjov commented Aug 20, 2018

Hey, @colinskow would be great if you find a time and accept this Pull-request, cause it still an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants